Add StringMap flag #1590

avorima · 2022-11-18T11:18:59Z

What type of PR is this?

feature

What this PR does / why we need it:

Add StringMap and StringMapFlag types that are backed by a map[string]string and parse comma-separated key=value pairs.

This is an analogue of pflag's StringToString.

Which issue(s) this PR fixes:

Fixes #1580

Special notes for your reviewer:

I mostly repurposed code from StringSlice. The tests were especially confusing to me since it seems a lot of it is duplicated.

Testing

Created a sample app that uses the flag.

package main

import (
	"fmt"
	"os"

	cli "github.com/urfave/cli/v3"
)

func main() {
	app := cli.NewApp()
	app.Flags = []cli.Flag{
		&cli.StringMapFlag{Name: "kv", EnvVars: []string{"KV"}},
	}
	app.Action = func(c *cli.Context) error {
		dict := c.StringMap("kv")
		fmt.Println(dict)
		return nil
	}

	app.Run(os.Args)
}

Release Notes

Add StringMapFlag that parses comma-separated key=value pairs

dearchap · 2022-11-28T00:05:13Z

@avorima the changes look good . Can you rebase off of 3.x since we are not adding new flags in 2.x anymore

avorima · 2022-11-28T10:32:08Z

@dearchap What branch is that, v3-dev-main?

dearchap · 2022-11-28T11:46:10Z

main

avorima · 2022-11-28T18:55:26Z

Code generation is somewhat broken by the changes to documentation in 1.19

avorima · 2022-12-02T08:38:27Z

Yeah, so there definitely is a difference when generating code with 1.18 and 1.19. I went with 1.19 now.

flag_test.go

dearchap · 2022-12-03T13:06:19Z

flag_map_impl.go

+	if reflect.TypeOf(t).Kind() == reflect.String {
+		return fmt.Sprintf("%v", v)
+	}
+	return fmt.Sprintf("%T{%s}", v, i.ToString(v))


Can you add a simple test case to test map[string]int to enable codecov of this line ?

I was thinking about this, but shouldn't that be added together with IntMap or similar?

Yeah you could do that.

dearchap · 2022-12-03T13:07:20Z

flag_map_impl.go

+		if !ok {
+			return fmt.Errorf("item %q is missing separator %q", item, defaultMapFlagKeyValueSeparator)
+		}
+		if err := i.value.Set(strings.TrimSpace(value)); err != nil {


would we ever need to trimspace for this kind of flags ?

It's easy to do something like ./prog --myflag "foo=$BAR" and BAR containing unwanted whitespace because it's a script output or whatever. On the other hand removing whitespace can also be delegated to the users of the library. How about I add a TrimSpace flag field that controls this behavior?

Sure that would be good. Thanks

Ok, I added it via the flag config. I had to add it to all string related flags to make the compiler happy

dearchap · 2022-12-03T13:08:31Z

flag_impl.go

@@ -192,7 +192,8 @@ func (f *FlagBase[T, C, V]) RunAction(ctx *Context) error {
 // IsSliceFlag returns true if the value type T is of kind slice
 func (f *FlagBase[T, C, VC]) IsSliceFlag() bool {


I think we need to rename this to IsMultiValueFlag as we are overloading this function name now . I can make that change in a separate PR.

@avorima PR #1615

StringMap wraps a `map[string]string` and parses inputs in the form of "key=value".

This config allows configuring to automatically trim whitespace of string value flags. This includes slices and maps. I had to plumb this down to the string flag level, because of compiler errors.

avorima force-pushed the add-string-map-flag branch from d32d928 to 841f091 Compare November 18, 2022 16:44

avorima force-pushed the add-string-map-flag branch 5 times, most recently from 4b5281d to 7466f42 Compare November 28, 2022 18:53

avorima force-pushed the add-string-map-flag branch 3 times, most recently from fe265f6 to fe9471c Compare December 2, 2022 08:35

avorima force-pushed the add-string-map-flag branch from fe9471c to ef880bd Compare December 2, 2022 08:51

avorima marked this pull request as ready for review December 2, 2022 08:56

avorima requested a review from a team as a code owner December 2, 2022 08:56

dearchap reviewed Dec 3, 2022

View reviewed changes

flag_test.go Show resolved Hide resolved

dearchap reviewed Dec 3, 2022

View reviewed changes

avorima force-pushed the add-string-map-flag branch 2 times, most recently from 5dd5dd0 to 2dac090 Compare December 5, 2022 22:05

avorima added 2 commits December 8, 2022 16:03

Add StringMap flag

9accc2a

StringMap wraps a `map[string]string` and parses inputs in the form of "key=value".

Add StringConfig for all string flags

e8f968a

This config allows configuring to automatically trim whitespace of string value flags. This includes slices and maps. I had to plumb this down to the string flag level, because of compiler errors.

avorima force-pushed the add-string-map-flag branch from 2dac090 to e8f968a Compare December 8, 2022 15:03

dearchap approved these changes Dec 9, 2022

View reviewed changes

dearchap merged commit 68a382f into urfave:main Dec 9, 2022

avorima deleted the add-string-map-flag branch December 13, 2022 11:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StringMap flag #1590

Add StringMap flag #1590

avorima commented Nov 18, 2022 •

edited

Loading

dearchap commented Nov 28, 2022

avorima commented Nov 28, 2022

dearchap commented Nov 28, 2022

avorima commented Nov 28, 2022

avorima commented Dec 2, 2022

dearchap Dec 3, 2022

avorima Dec 5, 2022

dearchap Dec 5, 2022

dearchap Dec 3, 2022

avorima Dec 5, 2022

dearchap Dec 5, 2022

avorima Dec 5, 2022

dearchap Dec 3, 2022

dearchap Dec 5, 2022

		@@ -192,7 +192,8 @@ func (f FlagBase[T, C, V]) RunAction(ctx Context) error {
		// IsSliceFlag returns true if the value type T is of kind slice
		func (f *FlagBase[T, C, VC]) IsSliceFlag() bool {

Add StringMap flag #1590

Add StringMap flag #1590

Conversation

avorima commented Nov 18, 2022 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Testing

Release Notes

dearchap commented Nov 28, 2022

avorima commented Nov 28, 2022

dearchap commented Nov 28, 2022

avorima commented Nov 28, 2022

avorima commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avorima commented Nov 18, 2022 •

edited

Loading